Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve code highlighting in web UI #4447

Open
wants to merge 64 commits into
base: main
Choose a base branch
from
Open

Conversation

pat-s
Copy link
Contributor

@pat-s pat-s commented Nov 23, 2024

@woodpecker-ci/maintainers as this might be another controversial design decision, please vote on the initial post of this PR with 👍 and 👎


This introduces a custom color. However, I found no color of the current palette to be matching here and/or creating a good contrast.
In addition, I slightly tweaked the padding and rounding values.

Before

image image

After

image image image

@pat-s pat-s added the ui frontend related label Nov 23, 2024
@pat-s pat-s added this to the 3.0.0 milestone Nov 23, 2024
@xoxys
Copy link
Member

xoxys commented Nov 23, 2024

Im not against changing the code block color but not this way. If we want a new one we should create a new color theme with --wp-code-{100,200,300}.

@xoxys
Copy link
Member

xoxys commented Nov 23, 2024

Keep in mind its used in various other places as well.

@pat-s
Copy link
Contributor Author

pat-s commented Nov 24, 2024

I don't think we need a full palette for a single light/dark code highlighting color.

I agree the naming is not optimal and doesn't fit into the current schema. Open for suggestions.

@xoxys
Copy link
Member

xoxys commented Nov 25, 2024

We dont need a full pallete right now we only have 100 and 200 just change the color of these.

@pat-s
Copy link
Contributor Author

pat-s commented Nov 26, 2024

100 and 200 are usually shades of bigger palette with 100 being the lightest value. We don't have a "real" palette for wp-code. Yes, two values exist

  --wp-code-100: theme('colors.int-wp-secondary.300');
  --wp-code-200: theme('colors.int-wp-secondary.600');

and yes, these again refer to another palette, but these values are also somewhat arbitrary and IMO not matching well in the UI. Hence I don't think that mimicking a palette is the best way forward here.

There should only be one color for wp-code ever IMO, for both light and dark. How about consolidating the two existing ones to one definition named wp-code? The two existing ones are only used in one instance in the code, respectively.

@xoxys
Copy link
Member

xoxys commented Nov 26, 2024

We are getting lost in discussions again. At the end, I don't really care, and wp-code works for me as well.

@pat-s
Copy link
Contributor Author

pat-s commented Nov 26, 2024

Hm, I think this was just a normal discussion? You brought up some reasons, I replied and shared my thoughts. It is not even about subjective UI but code syntax, so I'd say such discussions are normal (and needed and good).

@xoxys
Copy link
Member

xoxys commented Nov 26, 2024

Sorry this was not meant to be offensive or rude :) The color is only used in one location. While I have some personal preferences, I don't care much enough to discuss further how it is done in detail. wp-code works for me we can still change it later on if there is any need for a color palette. Sorry if my last message was misleading.

@pat-s pat-s added the build_pr_images If set, the CI will build images for this PR and push to Dockerhub label Dec 1, 2024
@pat-s
Copy link
Contributor Author

pat-s commented Dec 1, 2024

Consolidated all mixed wp-code definitions into wp-code-bg now, which aligns with the logic of wp-code-text.

@xoxys Is this acceptable in your view? Yes, the color is custom and not derived from the main colour palettes but I don't think that any value from these matches well here.

Need to run the PR image first to see in which places the changes trigger a change.

@xoxys
Copy link
Member

xoxys commented Dec 1, 2024

Fine for me.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 1, 2024

OK, my recent changes also change the bg color of the main log container. More work needed.

@xoxys
Copy link
Member

xoxys commented Dec 1, 2024

Ah yeah, I remember. That's why it's a bit questionable whether we really want to change the color, but I'm still fine if we want to change it only for real code blocks.

@pat-s pat-s modified the milestones: 3.0.0, 3.x.x Dec 1, 2024
@pat-s pat-s force-pushed the fix/code-highlight-colors branch 2 times, most recently from 15c182d to 994b797 Compare December 18, 2024 13:16
@pat-s pat-s marked this pull request as draft December 18, 2024 13:30
@pat-s pat-s marked this pull request as ready for review December 22, 2024 12:12
@pat-s
Copy link
Contributor Author

pat-s commented Dec 22, 2024

@xoxys You can take another look. I think I was able to keep all previous colors while achieving a much more consistent palette - tailwind mapping that should ease future UI changes.

@xoxys
Copy link
Member

xoxys commented Dec 25, 2024

Uhm the overall look and feel is still completely different and I dont like it:

image

image

@xoxys
Copy link
Member

xoxys commented Dec 25, 2024

image

On this view, the hover/non-hover color is inverted compared to the one in the screenshot above. Non-hover is white hover, is gray. This is IMO the correct behavior but should be used everywhere. The borders are way too dark now.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 25, 2024

Uhm the overall look and feel is still completely different and I dont like it:

I am actually confused why it looks like this now. The last time I checked it looked completely different.
Not sure, why maybe I did too many PRs in parallel.
Just to state explicitly, that this is not the look I indented.

@pat-s pat-s requested a review from xoxys December 26, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build_pr_images If set, the CI will build images for this PR and push to Dockerhub ui frontend related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants